-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for docker images #2714
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2714 +/- ##
==========================================
- Coverage 92.07% 91.93% -0.14%
==========================================
Files 169 170 +1
Lines 30054 30121 +67
==========================================
+ Hits 27672 27692 +20
- Misses 1762 1806 +44
- Partials 620 623 +3 ☔ View full report in Codecov by Sentry. |
Thank you for considering to support docker images! This looks very promising! As described in #724 (comment), we are using
It's a bit surprising that the pushed artifact isn't listed in the Zot-UI. Even after pushing the artifact, the UI still shows |
Attaching and downloading/verifying cosign-attestations (see #724 (comment)) works fine:
Or by digest:
Again, the attestation artifacts are not shown in the UI, unfortunately. But everything else seems to work! |
Docker manifest lists (media type $ regctl image copy alpine localhost:5003/library/alpine
WARN[0006] Failed to push manifest err="failed to put manifest localhost:5003/library/alpine:latest: request failed: unexpected http status code: Unsupported Media Type [http 415]: {\"errors\":[{\"code\":\"MANIFEST_INVALID\",\"message\":\"manifest invalid\",\"detail\":{\"description\":\"During upload, manifests undergo several checks ensuring validity. If those checks fail, this error MAY be returned, unless a more specific error is included. The detail will contain information the failed validation.\",\"mediaType\":\"application/vnd.docker.distribution.manifest.list.v2+json\"}}]}" target="localhost:5003/library/alpine"
Manifests: 8/9 | Blobs: 28.060MB copied, 0.000B skipped | Elapsed: 6s
failed to put manifest localhost:5003/library/alpine:latest: request failed: unexpected http status code: Unsupported Media Type [http 415]: {"errors":[{"code":"MANIFEST_INVALID","message":"manifest invalid","detail":{"description":"During upload, manifests undergo several checks ensuring validity. If those checks fail, this error MAY be returned, unless a more specific error is included. The detail will contain information the failed validation.","mediaType":"application/vnd.docker.distribution.manifest.list.v2+json"}}]} |
^ feature-ask-creep as cautioned ... "Properly" supporting this means UI changes, sync/mirror changes, cve scan changes, scrub changes, etc ... So now our turn, when can we expect these additional PRs from the community :) |
505af16
to
e6f5117
Compare
PR updated |
Just to be clear on expectations ... The plan is to initially include a config variable to support docker images for storage alone. |
https://github.com/dkowis/zot-builder I built the containers here using the same action as the regular zot repo. I haven't had a chance to try it yet, but I will soon. They're built to include this pull request. I am not quite skilled enough to tell it how to run every update to the pr |
I have pushed a docker buildx built container into zot. I can confirm that it doesn't show up in the UI anywhere, but a pull works just fine. |
^ a new "compat" config field is now added |
9a64b2d
to
b270eff
Compare
@andaaron let's merge this so community can at least start using zot as a docker registry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The storage package update is not complete. There is logic there going recursively from index to index to manifests to blobs.
Theoretically we could unmarshal the docker media types into oci structs for the purpose of handling GC and dedupe.
I think we can handle the search / CVE / UI changes in a separate PR, but the storage (GC/dedupe/scrub) changes should be included in this PR. |
597f77a
to
6d25346
Compare
Added more checks for this path, pls take a look. |
This is the same instance, a day later. Surely it would've updated by now? |
Yes, it should have. Is there anything special about that image? Is a manifest list with multiple manifests? What are the layer media types? Do you have multiple images in the same repository with different tags (that failed to scan message could be taken from the most recently built image pushed to the repository when you view the repository in the repository list) |
It is a pretty boring docker image. There should only be one tag. I'm not sure how to get the layer media types, but it's all built using docker. It should only be built for amd64. This is
|
I don't know how long it took for it to change that status, as I've been keeping my test build up to date for this thing. I'm going to assume that it's working fine, and maybe I got something in an intermediate state. I don't think we should worry about the scanning results in this Pull Request, if it shows up again, I'll file a new issue! Thanks! |
Issue project-zot#724 A new config section under "HTTP" called "Compat" is added which currently takes a list of possible compatible legacy media-types. https://github.com/opencontainers/image-spec/blob/main/media-types.md#compatibility-matrix Only "docker2s2" (Docker Manifest V2 Schema V2) is currently supported. Signed-off-by: Ramkumar Chinchani <[email protected]>
Garbage collection also needs to be made aware of non-OCI compatible layer types. Signed-off-by: Ramkumar Chinchani <[email protected]>
Signed-off-by: Ramkumar Chinchani <[email protected]>
Signed-off-by: Ramkumar Chinchani <[email protected]>
Signed-off-by: Ramkumar Chinchani <[email protected]>
Signed-off-by: Ramkumar Chinchani <[email protected]>
@dkowis can you rebase with the latest PR and try again. CVE scans may be fixed now, but would appreciate an independent confirmation. We would like to include this PR in the next -rc release so folks can try it. |
CVE scans do work. It shows "scan failed" shortly after uploading, but within like 30 seconds, it's changed to the vulnerability status, and I can see a list of vulnerabilities. I used v0.0.6 of my container here which is built using this branch. |
v2.1.2-rc3 is now released which should include this PR. |
Issue #724
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.